Combined fixes from contributor PRs 269, 272-276 - #286
Merged
Merged
Conversation
SendMultiTransactionBatch drains a wallet's size-1 error channel at most once before cancelling the batch. Every further sub-goroutine that failed blocked forever on its send, leaking the goroutine and its semaphore slot. Signal best-effort instead; the error is still recorded per transaction.
The confirmation path updated pendingTxCount with a load-then-store under txNonceMutex while GetNextNonce increments it under nonceMutex. A store computed from a stale load could roll the counter back and hand out an in-flight nonce twice.
The non-batcher funding path never recorded its transfers in batchTxMap, so the shared credit loop skipped them and the child wallets' tracked balance stayed stale after a successful transfer.
DeleteSpammer held the map write lock across Pause, which waits up to 10s for the scenario to wind down, freezing every reader meanwhile. Release the lock around Pause like DeleteGroup does and recheck the entry after reacquiring it.
OnComplete is documented as always being called, and ReclaimFunds relies on it to release its wait group. The early return for an already cancelled context skipped the callback, leaving the reclaim (and the spammer shutdown around it) hung forever.
Under EIP-8037 every fresh storage slot is charged 64 bytes of state creation gas on top of the SSTORE cost. A transaction at or below the EIP-7825 cap gets no state-gas reservoir, so that cost spills into regular gas and the fixed 370-address batch ran out of gas on every Amsterdam chain. Derive the batch size from the fee model's cost per state byte while keeping the gas limit at the EIP-7825 cap, so the transactions stay valid on chains that have not activated Amsterdam yet. The batch shrinks when a bloat tx still runs out of gas (the on-chain price can exceed the static estimate) and grows back after successful rounds. Nonces of transactions built for a round that is abandoned before sending are released.
This comment has been minimized.
This comment has been minimized.
SetNonce raised pendingTxCount with a load-then-store under nonceMutex, which does not exclude the confirmation path's update under txNonceMutex. A confirmation landing between its load and store could be overwritten with the lower on-chain nonce, reopening the clobber the previous commit closed. The root wallet resyncs its nonce every 48s, so this is reachable in normal operation.
This was referenced Sep 11, 2026
This comment has been minimized.
This comment has been minimized.
A hard error from SendMultiTransactionBatch left the nonces allocated by buildBloatTx permanently skipped when the tx never reached a node, so the next round built that wallet's tx at nonce+1 and could never get it included. A submission failure drops the tx from the wallet's pending tracking while a submitted-but-unconfirmed tx stays there, so use that to release only the nonces that are actually free. Also treat a nil receipt as a failed round instead of dereferencing it.
![redpandabot[bot]](https://avatars.githubusercontent.com/in/3749543?s=60&v=4){kind=small}
There was a problem hiding this comment.
Maintainer branch combining six contributor bug fixes: best-effort error signalling to stop the batch goroutine leak, a monotonic CAS for the pending nonce counter, funding credit for the non-batcher path, releasing the daemon spammer-map lock around Pause (with a post-reacquire recheck), calling OnComplete on an already-cancelled submission, and deriving erc20_bloater batch sizes from state-creation cost so bloat txs stay under the fixed gas limit on both pre-Amsterdam and Amsterdam chains. I traced every exit path (locks, channels, nonce tracking) and found the fixes sound: the first batch error still reaches the manager, OnComplete fires exactly once with a nil-guarded receipt, the CAS never moves pendingTxCount backwards, and the erc20 nonce-release never reuses an in-flight nonce.
Reviewed 11 changed file(s) @ 7a56f71e — no blocking issues found.
"I'm not a great programmer; I'm just a good programmer with great habits." — Kent Beck
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combined fix branch re-implementing the useful parts of the externally contributed PRs #269, #272, #273, #274, #275 and #276 on top of the current
txtypes-based master. Each PR describes a real bug in master; the fixes here were re-derived from the current code rather than rebased, and the tests exercise the real code paths instead of mirroring them.Fixes #260.
Changes
SendMultiTransactionBatchdrains a wallet's size-1 error channel at most once before cancelling. Every further failing sub-goroutine blocked forever on its send, leaking the goroutine and its semaphore slot. The signal is now best-effort; the error is still recorded per transaction.pendingTxCountwith a load-then-store undertxNonceMutexwhileGetNextNonceincrements it undernonceMutex. Replaced with a compare-and-swap loop that never moves the counter backwards.batchTxMap, so the shared credit loop skipped its transfers and the tracked balances stayed stale.DeleteSpammerheld the spammer map write lock acrossPause, which waits up to 10s. The lock is now released aroundPause(asDeleteGroupalready does) with a recheck after reacquiring it.ReclaimFundshang on cancelled context (Fix ReclaimFunds hanging forever when the context is cancelled mid-sweep #276):submitTransactionreturned early on an already cancelled context without firingOnComplete, although the callback is documented as always called.ReclaimFundsreleases its wait group only from that callback, so a shutdown or pause mid-reclaim hung forever.erc20_bloaterout of gas on Amsterdam (Fix erc20_bloater running out of gas on Amsterdam #269, [bug] erc20_bloater: bloat transactions revert with out-of-gas on Amsterdam (EIP-8037) #260): under EIP-8037 a transaction at or below the EIP-7825 cap gets no state-gas reservoir, so the 64 bytes of state-creation gas per fresh slot spill into regular gas. The fixed 370-address batch needs ~76M gas and reverted on every Amsterdam chain.erc20_bloater approach
#269 raises the per-tx gas above 16.7M to obtain a state-gas reservoir. That is rejected outright on Osaka chains that have not activated Amsterdam yet, which the PR compensates with string matching on
gas limit too highand a per-round fallback. It also sizes each tx to almost the full block gas limit regardless oftarget_gas_ratioand releases nonces of possibly in-flight transactions on any send error.This branch keeps the 16.7M limit and derives the batch size from the fee model's cost per state byte instead, so the transactions stay valid on both fork states without any retry logic:
--pre-amsterdam-fee-model)Because the on-chain price is dynamic and can exceed the static estimate, the batch shrinks by a quarter whenever a bloat tx still runs out of gas and grows back slowly after ten clean rounds. Nonces of transactions built for a round that is abandoned before sending are released so wallets do not develop a permanent gap.
Testing
go fmt,go vet,staticcheck(no findings in touched files) and the full test suite pass.OnCompletecontract drive the real code paths and fail against the previous code.erc20_bloatersizing is unit-tested but not yet validated on a Glamsterdam devnet.